PMM-14109 Improve Advisor UX - #5656
Draft
ademidoff wants to merge 152 commits into
Draft
Conversation
- Implemented MarkCheckResultsRead function in the service layer to update the read state of check results. - Added corresponding API endpoint for marking check results as read. - Introduced new parameters and response structures for the API. - Updated settings model to include Advisor notification settings. - Enhanced environment variable parsing to support new Advisor notification feature. - Added tests for the new functionality to ensure proper behavior.
…verity threshold
…d Advisor Notifications
…y Fields - Introduced CheckSeverity and CheckTriggeredBy types to represent the severity levels and initiators of advisor checks. - Updated CheckResult model to include Severity (CheckSeverity) and TriggeredBy (CheckTriggeredBy) fields. - Modified database schema to accommodate new fields in the check_results table. - Adjusted related services and models to handle the new fields, including changes in the execution flow of advisor checks. - Updated API services to support the new fields in requests and responses. - Enhanced tests to cover new functionality and ensure proper handling of severity and triggered by information.
… components - Introduced new fields: environment, cluster, replication_set, and outcome to CheckResultHistoryItem in advisors.proto. - Updated validation logic in advisors.pb.validate.go to include new fields. - Modified JSON representations in list_check_results_history_responses.go and v1.json to accommodate new fields. - Adjusted database schema and models to reflect new fields in CheckResult. - Enhanced tests to validate new fields and ensure proper functionality in AdvisorInsights components.
…, updating labels, and enhancing test coverage
ChangeSettingsRequest takes the recipient list as common.StringArray so an unset list can be told from one cleared to empty, while the Settings response returns a plain repeated string. toPayload round-tripped the response shape back to the API, so protojson rejected the bare array with "proto: syntax error: unexpected token [" and no advisor setting could be saved.
Both panes opened at 60vh unless a row double-click asked otherwise, so a pasted deep link (?details= or ?insight=) landed half-height and had to be maximized by hand every time. Each pane now opens maximized however it was triggered, and 60vh is reached only via the toggle; the choice is per-open and does not carry over. Drops the initialMaximized prop and the page-level state that fed it, and makes the toggle's aria-label follow its state.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Filters the list to the insights of one check, via the existing check_name support on ListInsights. No control was added to the filter bar: the table's first column is the check summary, so with the filter on every row shows the same check, and Clear filters already picks it up. MarkInsightsRead shares its filter object with the list query but takes InsightsFilters, which had no check_name — so the bulk mark-as-read would have ignored the new filter and marked every insight read, defeating the very guard that requires a filter before enabling the button. Added check_name there and wired it through to models.InsightFilters, which already applied it. InsightsFilters is unreleased (absent from main), so its fields are renumbered to put check_name first; no reserved ranges are needed.
The link competed with the row's own click targets and pushed the summary around. It becomes a non-clickable FileOpenOutlined indicator (a document with a north-east arrow) marking rows that have documentation; the working link already lives in the details pane, which the tooltip points at, and copy-as-text still carries the URL. Deliberately not OpenInNew: that glyph is the standard "opens in a new tab" affordance and would promise a click this indicator does not accept.
format-fast used $(FILES), which is empty unless passed explicitly, so a bare `make format-fast` died with "cannot use -w with standard input". $(GO_FILES) falls back to every *.go file, matching how `format` behaves; passing FILES="a.go b.go" still narrows it.
The interval spinner stepped by 0.1, so a click turned 24 into 24.1. step: 1 alone would not have fixed it: a number input anchors its steps to min, so the 0.1 floor made every stepped value fractional and would have left a plain 24 failing the browser's step check. The floor is now 1 hour, and the schema rejects typed fractions the spinner cannot produce. Intervals below an hour are no longer accepted by the form, so an install carrying one must correct that field before the tab can be saved. pmm-managed still accepts any interval down to a second; this is a UI floor only.
FileOpenOutlined's north-east arrow collapsed into something reading as a mouse cursor at the cell's ~16px font size, implying a click the indicator does not accept. DescriptionOutlined is the plain document glyph.
Record every Advisor checks execution in a new advisor_runs table so the chronological run history is browsable, and surface it as a Runs page. Totals are denormalized onto the run row on completion, derived from the insights it recorded, so a run keeps reporting accurate counts and a real wall-clock duration after its insights age out. Runs share the insight retention window but are pruned by their own start time, and a run left open by a restart is closed out at startup instead of reading as running forever. Also in this change, all on the same unreleased API surface: - Insights now carry the target's node/service/agent labels. Only the check script's own labels were stored, and those are empty for every built-in check, so the label chips the pre-insights page showed had silently gone missing. - Insights now populate the region and az columns, which already existed but were never written. The details pane reads Region from the column and no longer has a standalone AZ field. - Target.Copy dropped Environment, Cluster and ReplicationSet, so the per-database PostgreSQL targets recorded insights without them. - listInsights bypasses the axios case converter, which camelized the keys of the free-form label map, turning service_name into serviceName. - batch_id becomes run_id throughout, including the notification email and the copy-as-text output.
Fit all nine columns onto a laptop screen without horizontal scrolling, by sizing each column to its content and dropping the sort controls. Sorting was misleading anyway: the table pages server-side but sorted client-side, so a header click only reordered the page already loaded, and ListRuns takes no sort parameter. Also: - Keep Clear filters on screen at all times, disabled when there is nothing to clear, so the toolbar does not reflow. - Shorten two labels that were truncated or overlong: the Failed to run column is now Failed, and the row action is now View insights. - Move Checks ahead of Findings, so a run reads as how many checks ran and then how many found something. - Default to 50 rows per page.
The check errored with "index 0 out of range: empty list" whenever a metrics query returned no series, for example shortly after PMM Server starts. The existing guards covered an empty values array, not an empty series list, so docs[N][0] was indexed unconditionally. Missing metrics now yield an info insight naming the absent series instead of an error, and ahi_enabled is initialized for MySQL flavours that do not expose innodb_adaptive_hash_index. The METRICS_RANGE query is taken from the dashboard's "AHI Miss Ratio" panel, so it reports the share of searches NOT served by AHI, while the script treated it as a hit ratio: the low-hit-ratio insight fired when the hit ratio was good, and the contention branches required AHI to be barely used. The query is unchanged; the script now names the value missRatioAvg and compares it accordingly. Signed-off-by: Alex Demidoff <a@demidoff.me>
The check errored with "index 0 out of range: empty list" when any of its metrics queries returned no series, which is what happens shortly after PMM Server starts: the METRICS_INSTANT query for the redo log size is evaluated at a 5m lookback, so it has nothing to read yet. Missing metrics now produce an info insight naming the absent series. This also removes a division by zero: a series carrying no samples left cnt at 0, which the averages divided by. version and service are initialized so an empty first query cannot leave them unbound. Signed-off-by: Alex Demidoff <a@demidoff.me>
MySQL 8.0.30 sizes the redo log with innodb_redo_log_capacity and ignores innodb_log_file_size/innodb_log_files_in_group, which keep reporting their defaults. The check multiplied the legacy pair, so it under-reported the redo log size: measured on MySQL 8.4.10, 48M * 2 = 96M against an actual capacity of 100M, and the gap grows for anyone who sets capacity explicitly. The query now prefers the capacity variable and falls back to the legacy pair for older servers. The written and size values also went through int() on strings that increase() and avg() render as floats, which fails with "invalid literal with base 10"; they are parsed with int(float(...)) now. The missing-metrics insight no longer claims the gap is transient: checkpoint age comes from a Percona Server status variable that vanilla MySQL does not expose, so on those servers the condition is permanent. Signed-off-by: Alex Demidoff <a@demidoff.me>
Stakeholder review found the menu items under Advisors repetitive: the parent already says Advisors, so every child repeated the word. They are now Insights, Catalog and Run history. The page headings and document titles keep the Advisor prefix. There are no breadcrumbs, so the full name is what distinguishes a browser tab, and it keeps the canonical "Advisor checks" term visible on the page that the documentation, the tour and the email report all refer to by that name. The routing table in ui/AGENTS.md also gains the /advisors/runs row that was missed when the page was added. Signed-off-by: Alex Demidoff <a@demidoff.me>
Reviewing the catalog for the category rework surfaced metadata that
misdescribes the check on screen.
mysql_configuration_innodb_strict_mode carried the description of
mysql_security_password_lifetime ("This check warns about password
lifetime"), and that check in turn was summarized as "InnoDB password
lifetime". Both now describe themselves.
Three checks shared the summary "MySQL configuration check" and two more
shared "Server is not configured to enforce data integrity", so the
catalog listed them as indistinguishable rows. Each now names the
condition it reports, matching the wording of the insight it raises.
mysql_require_secure_transport also cited a variable that does not
exist: it reads require_secure_transport, not mysql_secure_transport_only.
Also fixes the "Unspported" typo in the mongodb_unsupported_version
summary.
Signed-off-by: Alex Demidoff <a@demidoff.me>
Removing the subcategory leaves the category as the only grouping, and the categories as authored do not survive that: Configuration held 66 of the 109 checks and said nothing about any of them, while Query held schema and index checks. Each check is now filed under what it actually examines: Security (27), Durability (12), Replication (11), Versions (11), Performance (11), Resources (11), Maintenance (7), Schema & indexes (7), Connections (6) and Logging (6). Grouping follows the risk rather than the feature, so for example sync_binlog is Durability rather than Replication, and the replication grant checks stay in Security. The subcategory field is dropped from every check file and from the three example templates. Signed-off-by: Alex Demidoff <a@demidoff.me>
With the catalog recategorized, the category groups the checks on its own and the second level only added a column to scan past. Subcategory is removed from the check format, the advisor_checks and advisor_insights tables, the API and the UI: the catalog loses its Sub category column and filter, the check form its field, and both details panes their row. An advisor group is now identified by its category alone. Migrations 119-121 are unreleased, so they are edited in place rather than followed by a drop-column migration. The four Prometheus check metrics labelled a check by its subcategory; the label is renamed to category and carries the category now. The only dashboard query on those metrics sums without a label selector. Removing the field left holes in three messages, so AdvisorCheck, Advisor and Insight are renumbered to close them, and the buf breaking baseline in api/descriptor.bin is refreshed accordingly. Signed-off-by: Alex Demidoff <a@demidoff.me>
The check name is the longest value on the pane and was truncating in a quarter-width column while Source, which holds either "Builtin" or "Custom", had a column to itself on the first row. Check name now spans two columns and Source moves down to sit with Interval and Status. Signed-off-by: Alex Demidoff <a@demidoff.me>
PMM has no SMTP configuration of its own: the sender reads the GF_SMTP_* variables it inherits from Grafana. Nothing in the UI told an operator whether those are set, so the only way to find out was to configure an unrelated Alerting contact point, or wait for a run to produce findings and hope the summary arrived. Settings -> Advisors now offers "Send test email" below the recipients field. It emails the addresses currently in the field, unsaved edits included, since verifying delivery is what the operator is about to rely on, and is disabled until they parse. The message is the real report: it goes through the same builder the scheduler uses, with one made-up finding per severity filtered by the configured threshold, so the recipient sees the format they will get. The subject is prefixed with [Test] and the body opens by saying the findings are samples and nothing was recorded, so a recipient who did not press the button cannot mistake them for real. Since SMTP cannot be fixed from this page, an unconfigured server has to say so: the sender wraps a new services.ErrSMTPNotConfigured naming the missing variable, and the endpoint returns it as a precondition failure. Signed-off-by: Alex Demidoff <a@demidoff.me>
The row menu already acts on one insight, whose service the surrounding items name, so "for this service" spent two words to say what the row already said. Signed-off-by: Alex Demidoff <a@demidoff.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket number: PMM-14109
Feature build: SUBMODULES-4478